-
Notifications
You must be signed in to change notification settings - Fork 6.9k
fix(windows): path handling fixes for Windows #6763
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
032638f to
61531f5
Compare
|
Hey! Your PR title Please update it to start with one of:
Where See CONTRIBUTING.md for details. |
adf2edd to
9b205c2
Compare
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
9b205c2 to
bef6a68
Compare
|
This sounds perfect @pschiel If you can get it ready and finish the cleanup you said I can test and approve. Btw don't put it behind an experimental flag. We'll ship once it's working. |
767f78b to
ba251fb
Compare
ba251fb to
aaa0f60
Compare
| @@ -1 +1 @@ | |||
| ../../ui/src/custom-elements.d.ts | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi this is a symlink.
on windows its disabled by default
enable it with
git config core.symlinks true
then recheckout all the files with
git checkout -- .
obviously that would reset any changes so stash/commit first.
that will fix the typecheck too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was intentional, as this requires admin permissions on windows.
thoughts: changing symlink into export statement in 2 files is worth it for that (the other ~70 symlinks are fonts/images and not affected by TS parsing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like this would've worked keeping as a simlink
/// <reference path="../../ui/src/custom-elements.d.ts" />
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@neriousy I went by this https://typescript-eslint.io/rules/triple-slash-reference/ which mentions triple slash being discouraged in modern ES6, and a rule for it.
please feel free to change, it was just so typecheck and tests run without errors, in a win32 environment
5d5db7d to
62152d4
Compare
62152d4 to
30a54ce
Compare
|
@Hona am through with it now, tidied up and ready to review |
|
Oh yeah, I was tackling it also today. Thought setting the cwd for the desktop app would be good when switching projects, but then you can switch the project with a session running in the background and it wouldn't work. I also realized that you'd need to refactor a lot of the app and this PR does it, relatively good. |
|
This would fix #9077 for example |
it's not yet documented - but that's what the "sandboxes" do. a worktree is the root of your git repo/worktree, a sandbox is a different directory inside. In the UI you see already the sandboxes, but they aren't explained, you don't see the path - looks like work in progress? |
Summary
This fix solves a whole series of bugs, resulting from filepath issues with backslashes occuring on Windows.
→ Solution: normalize all internal paths to forward slashes using centralized
FilesystemwrappersWhy this works: all win32 shells work with forward slash paths, this is de-facto industry standard (git, vscode, cmake, ... all use this pattern)
Observed issues
❌
git rev-parse --show-toplevelreturnsE:/x/yforward slash format → worktree/directory mismatch❌
path.resolve(),path.relative(),realpathSync.native(),realpath(bash) break with git bash paths (various issues)❌
/d/xwithin C drive (/c/) results in things likeC:\d\x❌ relative paths not working like expected using cross-drive
❌
containslogic not working → issues with permission system and external_directory❌
/tmpinside git bash is something else outside of it❌ tools break using backslash paths (some backslashes get "eaten")
❌ escaping hell for agents → requires double/quadruple escaping
❌ observed in all win32 native shells (git bash, cmd, powershell/pwsh)
❌ few issues in app/desktop (wrong filepath splits/usage)
Tested with fix
✅ local test suite succeeds now with 100% (
bun testinpackages/opencode)✅ all possible path variants (
C:\a\b,C:/a/b,/c/a/b,/cygdrive/c/a/b) are normalized intoC:/a/bformat✅ full functionality of all tools (
ENOENTerrors gone, permissions, relative paths, external directory, LSP)✅ worktree/sandbox handling correct
✅ tested TUI and desktop
How to reproduce/test
Notes
A few more places would require normalization - but they're unused/dead code (should be removed):
packages/ui/src/context/sync.tsxline 16absolute()packages/ui/src/context/local.tsxline 392, 420, 455, 546 - file tree operations (no file tree used)